Impersonating Troubleshooters can now impersonate users and groups#7465
Impersonating Troubleshooters can now impersonate users and groups#7465labkey-adam wants to merge 12 commits intodevelopfrom
Conversation
| .filter(Role::isAssignable) | ||
| .filter(role -> role.isApplicable(policy, c)) | ||
| .filter(role -> !role.isPrivileged() || canImpersonatePrivilegedRoles); | ||
| .filter(role -> !role.isPrivileged() || canImpersonatePrivilegedRoles) |
There was a problem hiding this comment.
Fix up mismatch between container used for c.hasPermission(ImpersonationPermission) and role.isApplicable(c)
There was a problem hiding this comment.
After further investigation, switching this to always pass in and check project (or root if null). AuthFilter checks perms based on the project stashed in the factory that's in session; it has no idea what the current folder is. That means we can impersonate roles only if they're applicable at the root or project level (never those only applicable in a folder). But no one has complained...
There was a problem hiding this comment.
Great, maybe add a comment to memorialize the thought?
There was a problem hiding this comment.
After further further investigation and testing, I went back to separate methods for listing roles that can be impersonated in the current context vs. checking permissions on each request while roles are being impersonated. So, getValidImpersonationRoles() again takes a container and returns roles applicable to that container (including folder roles) and verifyPermissions() again takes a (nullable) project. The main reason is that the impersonation factory we stash in session doesn't know where container impersonation was initiated; the _project it holds dictates the navigation restriction for project admins (or null for others). Without knowing the impersonation container we can't call isApplicable() on the roles... and this isn't important from a security standpoint. I did factor out a shared method for the primary permission check (has ImpersonatePermission in the appropriate container). Both methods ensure project admins can't impersonate root roles and app admins can't impersonate privileged roles. We could combine these methods in the future if we stash the impersonation container in the factory, but this isn't critical.
labkey-jeckels
left a comment
There was a problem hiding this comment.
One suggestion and two questions from automated code review tool. See what you think.
Rationale
https://github.com/LabKey/internal-issues/issues/837
Changes
ImpersonatePermissionclass is now used instead ofAdminPermissionas the primary check. Action-level and manager permission checking has been removed; all permission checking is performed by each impersonation context for simplicity and consistency.GroupImpersonationContext. This matches the behavior of when they impersonate a privileged user.AbstractRootContainerRole.isAvailableEverywhere()-->isApplicableOutsideRoot()CanImpersonatePrivilegedSiteRolesPermission-->ImpersonatePrivilegedSiteRolesPermissionwild-cardorwild card-->wildcard